Add search and skin tones to profile emoji picker - #4910
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The new picker assertions read overridden properties (48px control heights, the resting search background) immediately after the search input becomes visible. The Buzz stylesheet is appended from a `requestAnimationFrame` loop that only starts once `em-emoji-picker` has attached its shadow root, so it can land a frame or more later. An instrumented probe caught that gap: on 2 of 5 runs the shadow root had no `#buzz-emoji-mart-style` at that read point and the input measured Emoji Mart's un-overridden 36px instead of 48px. The shipped tests passed only because incidental round-trips bought enough time. `expect(...).toBe(48)` does not retry, so the race was live. Add `expectEmojiMartStylesInstalled` to the CSS helpers, polling for the style element, and gate both specs on it before their un-retried reads. The install loop itself predates this branch and is unchanged. Also documents the second probe finding, which needed no fix: the resting background samples as transparent right after install because Emoji Mart puts a 225ms `background-color` transition on the input. It settles to `rgb(229, 229, 230)`, and the override does win the cascade over the vendor `var(--em-color-border)` rule. Co-authored-by: kenny lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f8368d661
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf. The search/tone functionality and style synchronization look sound, but the increased non-modal picker height introduces a visible onboarding layout regression at the viewport shown in this PR's own screenshot. Please resolve the picker/CTA collision and add a layout assertion before landing.
| className="buzz-emoji-mart relative z-0 h-[316px] overflow-hidden rounded-xl bg-muted transition-colors duration-[250ms] ease-out" | ||
| className={cn( | ||
| "buzz-emoji-mart relative z-0 overflow-hidden rounded-xl bg-muted transition-colors duration-[250ms] ease-out", | ||
| isOnboardingModal ? "h-[316px]" : "h-[384px]", |
There was a problem hiding this comment.
P1 — Keep the onboarding CTA clear of the taller picker. AvatarStep uses the default presentation, so this branch increases its picker from 316px to 384px as well as adding the 68px search row. At the 1280×720 viewport in the PR's own onboarding screenshot, the picker ends around y=562 while the Next button starts around y=558; the picker visibly paints over the top of the CTA. That is a user-facing regression on a supported desktop-sized viewport, and the new E2E test currently locks in 384px without asserting that the actions remain visible and non-overlapping. Please make the height/layout responsive (or give this onboarding surface an appropriate presentation-specific height) and add a bounding-box/non-overlap assertion for the actions.


Summary
Testing
pnpm build:e2e